Skip to content

Add a windows sample#16339

Merged
wrowe merged 6 commits intoenvoyproxy:mainfrom
davinci26:win32examples
Jun 3, 2021
Merged

Add a windows sample#16339
wrowe merged 6 commits intoenvoyproxy:mainfrom
davinci26:win32examples

Conversation

@davinci26
Copy link
Copy Markdown
Member

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com

Fixes #13281

Risk Level: Low
Testing: Manual
Docs Changes: Added
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

@phlax I am adding this without verification first and I will follow up with an issue and a PR for the verification.

Also there is precedent with non verified examples wasm-cc is on the same boat.

@davinci26
Copy link
Copy Markdown
Member Author

cc @envoyproxy/windows-dev

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

@phlax the shellcheck is giving me a hard time with error but I am not sure why:

In ci/verify_examples.sh line 37:
    examples=$(find . -mindepth 1 -maxdepth 1 -type d -name "$TESTFILTER" ! -iname "_*"  $excluded_names | sort)
                                                                                         ^-- SC2086: Double quote to prevent globbing and word splitting.

Is there a bashy way to fix this?

@phlax
Copy link
Copy Markdown
Member

phlax commented May 6, 2021

Is there a bashy way to fix this?

not really if you mean autodiff (there is but it doesnt work well enough for even trivial use)

the error is pretty self-explanatory - add " around vars to prevent them unexpectedly splitting

@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented May 6, 2021

@phlax well the suggestion is wrong though. Adding "" makes the command to fail. Which is reasonable because I am intentionally splitting with multiple arguments in one string

Sotiris Nanopoulos added 2 commits May 7, 2021 14:42
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

@sunjayBhatia since you have a lot of operating experience, do you mind doing a review

Copy link
Copy Markdown
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM - very straightforward to follow. From an implementers' perspective @sunjayBhatia it would be good to get your thoughts before I proceed to merge. @phlax I trust your concerns are all addressed.

phlax
phlax previously requested changes May 17, 2021
Copy link
Copy Markdown
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we please warn users that the image is large and not open source

@pravb
Copy link
Copy Markdown

pravb commented May 17, 2021

@phlax for developers who are using or interested in Windows they already know that it is not open source so IMO no specific warning is needed. Envoy and Envoy Mobile is also supported on multiple other platforms that are based on non-open source software. We can certainly list the approximate image size in case the user is on a bandwidth constrained network.

Copy link
Copy Markdown
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just some small nits

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

@wrowe / @phlax any more outstanding comments?

Copy link
Copy Markdown
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wrowe wrowe dismissed phlax’s stale review June 3, 2021 23:58

Week has passed since review was re-requested, per discussion the remaining question appear moot, and could be addressed in that PR as needed

@wrowe wrowe merged commit ef0fb39 into envoyproxy:main Jun 3, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
* Adds a win32 example

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Envoy Windows server code samples

5 participants